Hey - I've found 1 issue

Please address the comments from this code review:

## Individual Comments

### Comment 1
<location path="src/codeweaver/engine/chunker/delimiter.py" line_range="474-477" />
<code_context>
+                continue

-                # Find the next structural opening after the keyword
-                struct_start, struct_char = self._find_next_structural_with_char(
-                    content,
-                    start=keyword_pos + len(delimiter.start),
-                    allowed=set(structural_pairs.keys()),
-                )
+            # Find the next structural opening after the keyword
</code_context>
<issue_to_address>
**suggestion (performance):** Avoid recreating the `allowed` set on each iteration by precomputing it once outside the loop.

Since `structural_pairs` doesn’t change within this function, compute `allowed_structurals = set(structural_pairs.keys())` once before the `for match in re.finditer(...)` loop and reuse it. This keeps the per-match path leaner, which matters on large files with many keyword hits.

Suggested implementation:

```python
            struct_start, struct_char = self._find_next_structural_with_char(
                content,
                start=keyword_pos + len(delimiter.start),
                allowed=allowed_structurals,

```

To fully implement the optimization, define the `allowed_structurals` set once before the `for match in re.finditer(...):` loop that contains this snippet. For example:

- Just above the loop (and after `structural_pairs` is defined), add:
  `allowed_structurals = set(structural_pairs.keys())`

Ensure that this variable is in the same scope as the loop so it is available where `_find_next_structural_with_char` is called.
</issue_to_address>

Pull request overview

Optimizes delimiter-based chunking by scanning the document once for all keyword-based delimiters, rather than running a separate regex scan per keyword delimiter.

Changes:

• Combine keyword delimiter starts into a single alternation regex and map match text back to a delimiter.
• Add an early return when keyword delimiters are empty after filtering.
• Fix docstring spelling (“normalised” → “normalized”).

💡 Add Copilot custom instructions for smarter, more guided reviews. Learn how to get started.

Review of PR #283: Combine keyword delimiters into single regex pattern

Summary

This PR optimizes the _match_keyword_delimiters method by combining multiple keyword delimiter patterns into a single regex using alternation. This is a smart optimization that reduces the complexity from O(K * N) to O(N), where K is the number of keyword delimiters and N is the document size.

Positive Changes

1. Performance Improvement: The optimization is well-motivated and should provide significant performance gains for large files with multiple keyword delimiters.

2. Code Quality:

   • Clear explanatory comment added (lines 465-466)
   • Early return optimization for empty delimiter lists (lines 454-455)
   • Logic remains functionally equivalent to the original

3. Documentation: Minor typo fix ("normalised" → "normalized") at line 1289

Critical Issue Found

⚠️ Bug: Dictionary key collision in delimiter_map

At line 467:

delimiter_map = {d.start: d for d in keyword_delimiters}

This dictionary comprehension will silently overwrite delimiters if multiple delimiters have the same start string. I found at least two cases where this occurs:

1. "type" appears in:

   • STRUCT_PATTERN (line 146: starts=["struct", "type"])
   • TYPE_ALIAS_PATTERN (line 161: starts=["type", "typedef", "typealias"])

2. "extension" appears in:

   • MODULE_PATTERN (line 153: starts=["module", "namespace", "package", "mod", "extension"])
   • EXTENSION_PATTERN (line 167: starts=["extension", "extend"])

Since ends="ANY" expands to empty strings (making these keyword delimiters), these patterns will have the same start but different kind, priority, or other properties. The current code will only keep the last one in the list.

Recommendation

The fix should handle duplicate start strings by either:

1. Use a list of delimiters per key:

   from collections import defaultdict
   delimiter_map = defaultdict(list)
   for d in keyword_delimiters:
       delimiter_map[d.start].append(d)

   Then in the loop, iterate over delimiter_map[matched_text] to create multiple matches.

2. Add validation to ensure no duplicates exist (if duplicates aren't expected by design)

3. Sort by priority and only keep the highest priority delimiter for each start string (if this is the intended behavior)

The original code didn't have this issue because it iterated over all delimiters individually, so each one was processed.

Test Status

• Most CI checks passing
• Python 3.14t experimental test failing (appears unrelated to this change)
• Recommend adding a test case that verifies behavior when multiple delimiters share the same start string

Verdict: ❌ Needs changes before merging due to the dictionary collision bug.

Found a critical bug where duplicate keyword start strings cause dictionary collisions. See my detailed review comment for full analysis and recommendations.

Code Review - PR #283

Summary

This PR optimizes the _match_keyword_delimiters method by combining multiple keyword delimiters into a single regex pattern, reducing complexity from O(K × N) to O(N) where K is the number of keyword delimiters and N is the document size.

✅ Strengths

1. Correct optimization approach: Combining multiple regex patterns using alternation ((?:pattern1|pattern2|...)) is a well-established optimization technique that reduces the number of passes over the input string.

2. Functionally equivalent: The logic correctly preserves the original behavior by:

   • Using match.group(0) to get the matched keyword text
   • Looking up the corresponding delimiter from delimiter_map
   • Maintaining the same filtering and structural matching logic

3. Performance improvement: This change will significantly improve performance for large files with many keyword delimiters, especially since the regex engine only scans the document once instead of K times.

4. Early exit optimization: The addition of the early exit check (lines 454-455) is a good defensive programming practice.

5. Minor typo fix: Fixed "normalised" to "normalized" for consistency with American English used elsewhere in the codebase.

⚠️ Potential Issues

1. Duplicate delimiter handling (Medium Severity)

The code uses a dictionary to map delimiter start strings to delimiter objects:

delimiter_map = {d.start: d for d in keyword_delimiters}

If multiple delimiters have the same start string, only the last one will be retained in the map. This could lead to:

• Lost delimiter configurations: If custom delimiters override built-in ones with the same start but different properties (kind, priority, etc.), only one will be used
• Inconsistent behavior: The behavior would depend on the order of delimiters in the list

Analysis: Based on the deduplication logic in _load_delimiters_for_language (lines 1381-1385), this scenario should be rare since the code checks for (start, end, kind) uniqueness. However, it's theoretically possible for keyword delimiters (which have empty end strings) to have the same start with different kind values.

Recommendation: Consider one of these approaches:

1. Add assertion: Add a debug assertion to verify no duplicate starts exist in keyword_delimiters
2. Use list comprehension: Keep only unique delimiter starts using a seen set
3. Document assumption: Add a comment documenting that duplicate starts are not expected

Example fix:

# Build delimiter map - ensure no duplicate starts (each start should be unique)
seen_starts = set()
unique_delimiters = []
for d in keyword_delimiters:
    if d.start not in seen_starts:
        seen_starts.add(d.start)
        unique_delimiters.append(d)
delimiter_map = {d.start: d for d in unique_delimiters}

2. Regex pattern efficiency with duplicates (Low Severity)

If there are duplicate starts in keyword_delimiters, the combined pattern will contain redundant alternatives:

combined_pattern = rf"\b(?:{'|'.join(re.escape(d.start) for d in keyword_delimiters)})\b"
# Could produce: \b(?:function|function|def)\b

This is inefficient but functionally correct. The same deduplication fix above would address this.

📝 Minor Observations

1. Comment quality: The optimization comment (lines 465-466) clearly explains the benefit. Well done!

2. Code style: The change maintains consistency with the surrounding code style.

3. Testing: The PR description mentions running tests via uv run pytest tests/unit/engine/chunker. The existing integration tests should catch any behavioral regressions.

🎯 Recommendation

Approve with minor suggestion: The optimization is sound and provides real performance benefits. The potential duplicate handling issue is theoretical and may not occur in practice due to existing deduplication logic. However, I recommend adding either:

• A comment documenting the assumption that keyword_delimiters contains unique start strings, OR
• Defensive code to deduplicate by start string

The CI failure for Python 3.14t appears to be experimental and can be safely ignored based on the workflow configuration (continue-on-error: true for experimental versions).

Verdict

✅ LGTM with minor suggestion - The optimization is correct and valuable. Consider adding duplicate handling or documentation for robustness.